Entra ID integration: add plugin boilerplate and OIDC adjustments by justinas · Pull Request #40556 · gravitational/teleport

justinas · 2024-04-15T15:36:43Z

~~This targets an Entra ID feature branch.~~ Part of https://github.com/gravitational/access-graph/issues/625 . Enterprise counterpart is https://github.com/gravitational/teleport.e/pull/3965

This is the first PR which introduces the proto types for Entra ID plugin. Additionally, together with its enterprise counterpart PR, this implements authentication to the Azure APIs via federated credentials, i.e. OIDC.

A similar approach was taken as with our AWS OIDC integration. However, I had to adjust how we sign our JWTs and present the JWKS, as the behavior is inconsistent across implementors:

AWS requires a kid property for a JWK in JWKS, but supports the value being an empty string. It does not require a kid claim in the header of JWT.
Microsoft requires a kid header claim in the JWT, and requires it to be non-empty. It also wants to match the value of this property to a JWK in the JWKS.

Currently we present JWKs with kid: "" in our JWKS, which is not suitable for Microsoft. I first introduced Key IDs for synthetic names (derived from the public key) for our keys. However, to not break the AWS OIDC integration, I also needed to have the key duplicated twice in JWKS: once with "", and once with a non-empty key "some-base64-value", as well as having to add a header claim with the empty kid.

We could take a cleaner approach of always having non-empty Key IDs (both in JWTs and in JWKS), but this would require manual intervention from users - we now point users to set up their JWKS and OIDC configuration in an S3 bucket, which we do not have persistent access to. So they would need to re-enroll the integration once again.

I manually tested this for regresssions to the best of my ability with the AWS OIDC integration:

Setting up the integration with an existing version of Teleport, and migrating to this version.
Setting up the integration with this version.

- Synthesize Key IDs for our JWT keys. For backwards compatibility, also include the same keys with an empty `kid` in JWKS. - Sign AWS OIDC tokens with a `kid=""` header claim, rather than omitting the `kid` claim altogether. See comment for details.

github-actions · 2024-04-15T20:13:15Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

jakule · 2024-04-17T16:24:21Z


 // signAny will return a signed JWT with the passed in claims embedded within; unlike sign it allows more flexibility in the claim data.
-func (k *Key) signAny(claims any) (string, error) {
+func (k *Key) signAny(claims any, opts *jose.SignerOptions) (string, error) {


nit: Instead of passing nil in all places where options don't need to be set, you could let pass WithHeader(Jose.HeaderKey("kid"), ") as ...options, so if not needed it can be skipped.

I don't think that can be easily done here, as options are methods on the SignerOptions struct and do not follow the "functional options" pattern 😕

You can totally do that. You just need to write a bit of boiler plate to create a bunch (in our case 1) apply function that you can then apply to SignerOptions.

tigrato · 2024-04-17T18:26:23Z


+// PluginEntraIDSettings defines settings for the Entra ID sync plugin
+message PluginEntraIDSettings {
+  option (gogoproto.equal) = true;


Don't use gogo, use derive to generate the equal funcs

Do we want to use it as a plugin or do we want to use it as an integration similarly to AWS?
I think the second is preferred as it's similar to aws

Do we want to use it as a plugin or do we want to use it as an integration similarly to AWS?
I think the second is preferred as it's similar to aws

That's a good question. I looked into it, and I'm not fully sure what the role of an Integration is. Plugin came first, then Integration was created at some point. There is only a single Integration, that is AWS OIDC, but many Plugins. Integrations like Okta and Jamf run as either:

An independent service

A Plugin.

and I would argue they are closer to what we're implementing for Entra for now, as we're importing RBAC resources and not compute resources that the AWS integration mostly focuses on.

One benefit is that hosted plugins are now enabled for any enterprise deployment and the use of them does not require to spin up a discovery service instance.

Don't use gogo, use derive to generate the equal funcs

I'm afraid all Plugin types are using gogoproto.equal currently, do we want to deviate from that at this point?

I was thinking about the same, but then Okta is also defined as a plugin 🤷‍♂️

@r0mant ?

I'm still failing to fully grasp the distinction, but I'll defer my judgement (let's see if Roman has any input).

Integration + Discovery Config workflow seems to have some rough UX edges, see https://github.com/gravitational/access-graph/issues/731 , though they are probably solvable.

Had some time to think about this.

Entra being run as either a Plugin or a standalone host follows the established pattern for Jamf and Okta, as well as the upcoming GitLab TAG integration.

If we'd like to utilize the same "integration" credentials to discover Azure resources (which is out of scope for now), we only need Auth server to be able to utilize its information for signing the OIDC tokens - it should be able to do that whether the info is in an Integration or a Plugin.

IMO having this as a plugin is the most straightforward way. Let me know if I'm missing any hard blockers for this approach. @tigrato @jakule

No preferences as long as it works really. @tigrato ?

Can't we simply use an integration to provide access to Entra/Azure OIDC token - it's what an integration does as it doesn't run any code - while the sync code runs as a plugin. This would simplify future changes with supporting Azure discovery in the cloud as we don't need to maintain 2 pieces of code for the same thing.

My suggestion is:

add an integration: provides dynamic credentials signed by teleport to be used for azure/entra access

add a plugin: leverages the integration to generate the OIDC credentials it needs to access Entra and polls access lists from it and syncs to Teleport.

@tigrato @jakule I've updated the PR by introducing an Azure OIDC integration type and moved the auth-related stuff there.

I did not expose a method for discovery to acquire OIDC tokens - will do that when we get there. For the plugin it is much easier to do that directly through Auth.

…oilerplate

justinas · 2024-04-29T13:53:55Z

Split into #40997 and #40998 per request.

Justinas Stankevicius added 10 commits April 15, 2024 17:49

Add Entra ID plugin boilerplate

8738331

Add e imports for MS Graph SDK

2c3ce67

Restore proto stuff lost in merges

55ea750

Add unit tests for Entra ID plugin validation

8c96fcb

Fix typo in assertion function name

985bac0

Update the OIDC JWKS test to expect the same key twice

a693e43

Add Entra ID plugin type constant

aee903d

go mod tidy

8670925

Fix expected JWKS size in integration test

ebb1288

justinas marked this pull request as ready for review April 15, 2024 20:12

github-actions Bot added the size/md label Apr 15, 2024

github-actions Bot requested review from capnspacehook and ryanclark April 15, 2024 20:13

justinas added the no-changelog Indicates that a PR does not require a changelog entry label Apr 16, 2024

justinas requested review from jakule and tigrato and removed request for ryanclark April 16, 2024 12:09

justinas changed the base branch from justinas/entra-id-feature to master April 17, 2024 14:07

Merge branch 'master' into justinas/entra-id-boilerplate

d6c698e

jakule approved these changes Apr 17, 2024

View reviewed changes

tigrato reviewed Apr 18, 2024

View reviewed changes

Add basic tests for KeyID

7f8551a

justinas requested review from jakule and tigrato April 22, 2024 17:33

Move Azure auth settings from Plugin to Integration

866a4e2

justinas force-pushed the justinas/entra-id-boilerplate branch from 8cef2af to 866a4e2 Compare April 24, 2024 17:56

Merge remote-tracking branch 'origin/master' into justinas/entra-id-b…

1837b94

…oilerplate

justinas mentioned this pull request Apr 29, 2024

Entra ID integration: add proto definitions #40997

Merged

justinas mentioned this pull request Apr 29, 2024

Entra ID integration boilerplate #40998

Merged

justinas closed this Apr 29, 2024

Conversation

justinas commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinas commented Apr 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justinas commented Apr 15, 2024 •

edited

Loading